Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds demo for plotting sensors #79

Merged
merged 3 commits into from
Dec 8, 2023
Merged

Conversation

neukym
Copy link
Member

@neukym neukym commented Dec 7, 2023

Starts fixing #78

@neukym neukym requested a review from caiw December 7, 2023 19:01
@neukym neukym added ready-for-merging 📈 plotting functionality Any issues related to plotting and removed ready-for-merging labels Dec 7, 2023
@neukym
Copy link
Member Author

neukym commented Dec 7, 2023

Looks good @caiw, but there are a couple of issues:
Screenshot 2023-12-07 at 19 02 55

  1. The x axis is shrunk
  2. The hemisphere are mirrored

Could I leave with you to fix both of these?

@caiw caiw self-assigned this Dec 8, 2023
@caiw
Copy link
Member

caiw commented Dec 8, 2023

Oops yes I'll take a look

@caiw
Copy link
Member

caiw commented Dec 8, 2023

  1. is because source data latency is in seconds whereas sensor data latency is in milliseconds, and the axis xlims are being set manually to [-200, 800]... @neukym Shall we decide on a standard time unit?

@neukym
Copy link
Member Author

neukym commented Dec 8, 2023

I don't know. Second is the SI unit, but milliseconds is what we use day-to-day, and it always takes me a moment or so to mentally translate "0.045" into 45 milliseconds. I vote milliseconds.

@caiw
Copy link
Member

caiw commented Dec 8, 2023

[2. is now fixed]

@caiw
Copy link
Member

caiw commented Dec 8, 2023

I don't know. Second is the SI unit, but milliseconds is what we use day-to-day, and it always takes me a moment or so to mentally translate "0.045" into 45 milliseconds. I vote milliseconds.

Yeah I could go either way. I think we should definitely show milliseconds on x-axes, but I can also see an argument for using the SI unit in the datastructure. What does MNE do?

@caiw
Copy link
Member

caiw commented Dec 8, 2023

One argument in favour of seconds is that all the .nkg hexel files we currently have use seconds.

@caiw
Copy link
Member

caiw commented Dec 8, 2023

Anyway the source of bug (1.) is the different time units on the sensor and hexel .nkg files, not in the code (although manually setting the x-ticks is something we'll have to address eventually when using the next dataset). I suggest we wait until we've decided on a standard time unit and then get the sample datasets all to use that unit and then verify the plots look good before we merge this in.

@caiw
Copy link
Member

caiw commented Dec 8, 2023

Thanks for your reviews @neukym! Catching my bugs :)

@caiw caiw merged commit 1cbe7c3 into main Dec 8, 2023
1 check passed
@caiw caiw deleted the adds_demo_for_plotting_sensors branch December 8, 2023 09:40
Copy link
Member

@caiw caiw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@neukym
Copy link
Member Author

neukym commented Dec 8, 2023

Thanks for your reviews @neukym! Catching my bugs :)

The system works! Merging now.

@neukym
Copy link
Member Author

neukym commented Dec 8, 2023

Oh you've merged :-)

@neukym
Copy link
Member Author

neukym commented Dec 8, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📈 plotting functionality Any issues related to plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants